cc @theuni

@theuni @purpleKarrot Can one of you review this?

I'd rather drop the complete block of code. Every CMake project should behave well as a subproject. This becomes even more relevant with FetchContent getting more and more adoption. Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache), they should also not define their own settings that already have a default in CMake. Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don't have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

The CMake cache is global in scope

Sigh, this is again confusing as hell (at least to me as someone who is not a CMake expert). Let me try to organize the discussion.

What is the cleanest thing to do in a library?

@hebasto:

According to CMake’s documentation, [...]

@purpleKarrot:

Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache) [...]

The docs say that top-level projects should call option(BUILD_SHARED_LIBS ...). Do we agree that, under the assumption that we are in control of both the top-level project and the subproject, this is only the second-best thing? And the best thing to do is to avoid setting this variable in the subproject?

Can we drop the entire block?

I might be wrong, but wouldn't dropping the option(BUILD_SHARED_LIBS ...) here mean that this variable can't be set on the command line using -D (because it's not even a cache variable then)? At least, this is how I read the docs.

@purpleKarrot:

they should also not define their own settings that already have a default in CMake.

The thing is that, according to the docs, BUILD_SHARED_LIBS does not have a default.

If we can drop the entire block, should we do it?

I'm not sure, but I tend to say no. I'd rather keep the default of building a shared library and accept a few lines of CMake code. If we decide to drop it, we should at least document this in the README.

Do we need SECP256K1_DISABLE_SHARED?

Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don't have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

I had to look this up. The reason why we have SECP256K1_DISABLE_SHARED is this:

@theuni in #1230 (comment):

A new BUILD_SHARED_LIBS option is added to match CMake convention, as well as a SECP256K1_DISABLE_SHARED option which overrides it. That way even projects which have BUILD_SHARED_LIBS=1 can opt-into a static libsecp in particular.

@purpleKarrot
Are you saying this is not necessary? (Real question, I really don't understand all of this stuff.)

The CMake cache is global in scope

Sigh, this is again confusing as hell (at least to me as someone who is not a CMake expert). Let me try to organize the discussion.

What is the cleanest thing to do in a library?

@hebasto:

According to CMake’s documentation, [...]

@purpleKarrot:

Not only should projects not interfere with superprojects (like setting BUILD_SHARED_LIBS or CMAKE_BUILD_TYPE in the cache) [...]

The docs say that top-level projects should call option(BUILD_SHARED_LIBS ...). Do we agree that, under the assumption that we are in control of both the top-level project and the subproject, this is only the second-best thing? And the best thing to do is to avoid setting this variable in the subproject?

This excerpt from the docs literally describes how fragile it is to set BUILD_SHARED_LIBS as a cache variable:

... if bringing external dependencies directly into the build ... and one of those dependencies has such a call to option(BUILD_SHARED_LIBS ...), the top level project must also call option(BUILD_SHARED_LIBS ...) before bringing in its dependencies.

Additionally, from Professional CMake: A Practical Guide 21st Edition, Section 40.2:

Another common problem is modifying variables that have the potential to affect the whole build, not just the project... Quite often, these variables [BUILD_SHARED_LIBS, ...] shouldn’t be defined by the project as cache variables at all...

Can we drop the entire block?

I might be wrong, but wouldn't dropping the option(BUILD_SHARED_LIBS ...) here mean that this variable can't be set on the command line using -D (because it's not even a cache variable then)? At least, this is how I read the docs.

When set using -D, the variable becomes a cache variable.

@purpleKarrot:

they should also not define their own settings that already have a default in CMake.

The thing is that, according to the docs, BUILD_SHARED_LIBS does not have a default.

The default state of BUILD_SHARED_LIBS is "undefined". And the behaviour of the add_library command is well-defined in this case.

If we can drop the entire block, should we do it?

I'm not sure, but I tend to say no. I'd rather keep the default of building a shared library and accept a few lines of CMake code. If we decide to drop it, we should at least document this in the README.

I agree with that. That's why I chose the suggested approach.

A shared library is a real library and should be considered as the default by-product of the build process.

Do we need SECP256K1_DISABLE_SHARED?

Managing a superproject is much easier when you can rely on the assumption that projects can be controlled through standard CMake variables and you don't have to remember separate names like FOO_BUILD_SHARED, BAR_NO_SHARED, BAZ_DISABLE_STATIC, etc.

I had to look this up. The reason why we have SECP256K1_DISABLE_SHARED is this:

@theuni in #1230 (comment):

A new BUILD_SHARED_LIBS option is added to match CMake convention, as well as a SECP256K1_DISABLE_SHARED option which overrides it. That way even projects which have BUILD_SHARED_LIBS=1 can opt-into a static libsecp in particular.

@purpleKarrot Are you saying this is not necessary? (Real question, I really don't understand all of this stuff.)

Using project-specific versions of well-known CMake variables is hard to maintain consistent. This project defines its own variable to override CMake's BUILD_SHARED_LIBS, but omits others such as CMAKE_EXPORT_COMPILE_COMMANDS. Specifying the library type is not the only concern when integrating into a parent project. In Bitcoin Core, we currently use an approach that encapsulates the entire integration logic within a function.

Real question, I really don't understand all of this stuff.

I hear you, @real-or-random. That deserves a longer explanation. I will provide extensive information with examples on the weekend.

I hope to answer most of your questions here: https://njump.me/nevent1qqsyfmk89g3cfvq8u9p6hg3x4nd0j66tjgwkth45wpz493hv4f5z70spp4mhxue69uhkummn9ekx7mqzyq8sv8lyu6zjy5p8fmfa0pn4j95rvmr2rj7urnpdhvkk5r55fndjkd3z5dj

That's a great post!

What's your conclusion for this PR? From the post, it appears that you support the if(PROJECT_IS_TOP_LEVEL) approach as currently implemented in this PR. But you said above that you'd rather drop the complete block of code. Did you change your mind when writing the post?

The point of the __GNUC__ is to check that the compiler supports __attribute__((visibility("default"))) at all. You wouldn't want to set it on some non-GCC and non-Clang compiler.

Note that modern gcc/clang support has_attribute. It can be used like this:

#if defined(__has_attribute)
#  if __has_attribute(visibility)
#    define HAS_VISIBILITY_ATTRIBUTE
#  endif
#endif
#ifdef HAS_VISIBILITY_ATTRIBUTE
#  define DEFAULT_VISIBILITY_ATTRIBUTE __attribute__((visibility("default")))
#else
#  define DEFAULT_VISIBILITY_ATTRIBUTE
#endif

As illustrated here.

Of course to be useful for libsecp it'd need a fallback for older versions.

That's a great post!

Thanks! It was a huge effort.

What's your conclusion for this PR? From the post, it appears that you support the if(PROJECT_IS_TOP_LEVEL) approach as currently implemented in this PR. But you said above that you'd rather drop the complete block of code. Did you change your mind when writing the post?

In my post, I show several approaches and possibilities.

Project Qux sets BUILD_SHARED_LIBS as an option based on Qux_IS_TOP_LEVEL, similar to this PR. I prefer to use project specific variables in CMakeLists.txt files and use generic variables like PROJECT_IS_TOP_LEVEL in generic code.

In project Bar, I show that no such option is actually needed for building with both BUILD_SHARED_LIBS=ON and BUILD_SHARED_LIBS=OFF.

In project Baz, I show that even with BUILD_SHARED_LIBS globally set to ON, it is possible to set it to a different value for a subproject, without there being a QUX_DISABLE_SHARED option.

For this PR, it means I would definitely drop the else part, and preferably the complete block of code.

Note that modern gcc/clang support has_attribute.

Clang supports visibility since version 2.0 and __has_attribute since version 2.9. How should __has_attribute(visibility) ever be false? Maybe for targeting embedded platforms, but you would not want to build shared libraries for those.

I am aware of 1309c03, but I cannot tell whether there is a actual use case to support compilers older than bitcoin, or whether that change was motivated by because-we-can.

But yeah, that is off-topic for this PR. Please use nostr to comment on the blog post.